Skip to content

fix(presets): recompose surviving skills when a lower preset layer is removed - #5013

Open
yxshee wants to merge 2 commits into
github:mainfrom
yxshee:fix/preset-remove-stale-composed-skill
Open

yxshee wants to merge 2 commits into
github:mainfrom
yxshee:fix/preset-remove-stale-composed-skill

Conversation

@yxshee

@yxshee yxshee commented Oct 17, 2026

Copy link
Copy Markdown

Description

Removing a preset that is the base, or a middle layer, of another preset's prepend/append/wrap composition leaves the removed preset's content in the active agent's SKILL.md. That affects Claude, Codex, Copilot with --skills, and other skills-based integrations. specify preset resolve reports the new chain, and command-file agents such as Gemini are recomposed onto it. But the skill keeps the removed preset's instructions, so /speckit-specify runs content the user just removed.

Reproduction (alpha replaces speckit.specify with ALPHA BODY; beta appends BETA APPENDED):

$ specify init p --integration claude --ignore-agent-tools --non-interactive && cd p
$ specify preset add --dev ../alpha --priority 5
$ specify preset add --dev ../beta --priority 2
$ specify preset remove alpha
✓ Preset 'alpha' removed successfully
$ grep -c "ALPHA BODY" .claude/skills/speckit-specify/SKILL.md
1          # main; 0 with this change
$ specify preset resolve speckit.specify
    (top layer from: beta v1.0.0)
    1. [base] core (bundled) → ...
    2. [append] beta v1.0.0 → ...

The same steps with --integration gemini drop ALPHA BODY from .gemini/commands/speckit.specify.toml on main, so only skills output is stale.

Root cause, in presets/_manager_skills.py:

The fix:

  • _reconcile_skills() now records commands whose winning layer composes (strategy other than replace).
  • After a removal, it also re-renders those skills in the active directory, but only when the SKILL.md still carries the winning preset's metadata.source: preset:<id> marker. A new helper, _skills_owned_by_preset(), does that check.
  • A hand-edited skill is left alone, matching the existing protection for non-owned skills during removal.
  • The install and rescaffold paths are unchanged.

Testing

  • Tested locally with uv run specify --help
  • Ran existing tests with uv sync && uv run pytest
  • Tested with a sample project (if applicable): the reproduction above with claude (on main and this branch) and gemini (on main)

New tests in tests/specify_cli/presets/test_manager_skills.py:

  • test_remove_base_preset_recomposes_surviving_composed_skill: fails on main with assert "Base preset body" not in content
  • test_remove_middle_preset_drops_its_layer_from_composed_skill: fails on main with assert "Middle body" not in content
  • test_remove_base_preset_preserves_hand_edited_composed_skill: the negative case, guarding the hand-edit protection; passes before and after

With the change the file gives 70 passed.

  • Preset, integration, bundle and extension suites: 6353 passed, 9 skipped.
  • Full suite (macOS, Python 3.13): 12332 passed, 376 skipped. 11 failures in the tests/test_*_python_parity.py files occur identically on unmodified main on this machine under full-suite load; those files pass when run on their own (257 passed, 112 skipped).
  • One more local failure, test_resolver.py::TestResolveCore::test_resolve_core_does_not_return_preset_files, was an artifact of a worktree directory literally named presets: the test asserts "presets" not in path.parts. It fails identically on main in that directory and passes elsewhere.
  • uvx ruff@0.15.0 check src tests: clean

AI Disclosure

  • I did not use AI assistance for this contribution
  • I did use AI assistance (fill in the disclosure below)

AI disclosure: Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode) opened this PR on behalf of @yxshee. A Claude Code subagent (same model, autonomous) found the bug and wrote the fix and tests. The lead agent independently re-ran the new tests against main and the branch, reproduced the behavior with the real CLI (Claude and Gemini), confirmed the #3459 origin with git log -S, reviewed the diff, and wrote this description. No human reviewed the change line by line before it was opened.

🤖 Generated with Claude Code

… removed

Removing a preset that is the base (or a middle layer) of another preset's
prepend/append/wrap composition left the removed preset's content in the
active agent's SKILL.md (Claude, Codex, Copilot with --skills, ...), while
command-file agents such as Gemini were recomposed onto the new stack.

The composed skill is owned by the surviving preset at the top of the
chain, so _unregister_skills() restores nothing for the removed preset,
and _reconcile_skills() only revisited the active skills directory when
that restore had touched it. The recomposed .composed/<cmd>.md that
_reconcile_composed_commands() writes was therefore never rendered into
the skill, and `specify preset resolve` disagreed with what the agent ran.

After a removal, also reconcile active-directory skills for commands whose
winning layer composes, as long as the skill still carries the winning
preset's source marker. A hand-edited skill is left alone, matching the
existing protection for non-owned skills during removal.

Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@yxshee
yxshee requested a review from mnriem as a code owner October 17, 2026 19:15
@mnriem mnriem added the triage-must-have Verdict: high-value, important work for Spec Kit — do first label Oct 18, 2026
@mnriem
mnriem requested a max review from Copilot October 19, 2026 13:03

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Removal can overwrite a user-owned legacy skill despite the ownership filter.

1 open issue
What changed in this PR

Updates Spec Kit preset removal so surviving composed skills reflect the remaining preset layers.

Changes:

  • Re-renders active composed skills that retain the winning preset’s ownership marker.
  • Adds regression coverage for base/middle removal and preservation of non-owned skills.

Validation used an isolated ownership check; project tests were not run.

AI disclosure: GitHub Copilot (model: unknown, autonomous) generated this review for the requesting reviewer.

File Description
tests/​specify_cli/​presets/​test_manager_skills.py Adds composition-removal and preservation tests.
src/​specify_cli/​presets/​_manager_skills.py Adds ownership-based reconciliation for surviving composed skills.

Review effort: Max

Comment on lines +406 to +408
active_names |= self._skills_owned_by_preset(
active_skills_dir, composed_winners
)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed and fixed in 8c0328f. _register_skills() now takes an optional only_skill_names filter; on removal the active directory passes its managed names, so a user-owned legacy speckit.specify/SKILL.md next to a recomposed speckit-specify is no longer rewritten. Install and rescaffold pass None and are unchanged. New regression: test_remove_base_preset_preserves_user_owned_legacy_skill, which fails on 1239cbe and passes now.

AI disclosure: posted on behalf of @yxshee by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the fix, test and this reply are AI-generated.

🤖 Addressed by Claude Code

When both the modern (speckit-specify) and legacy (speckit.specify)
skill directories exist, the active-directory recomposition added for
lower-layer removal selected the command because the modern skill kept
the winning preset's marker, and _register_skills() then rewrote both
directories, overwriting a user-owned legacy skill.

Give _register_skills() an optional only_skill_names filter and pass the
directory's managed names on removal, so only names that are still
preset-owned are written. Install and rescaffold calls pass None and
keep writing every existing directory, as before.

Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@yxshee

yxshee commented Oct 19, 2026

Copy link
Copy Markdown
Author

Review round 1, posted on behalf of @yxshee.

Addressed the Copilot finding that removal recomposition could overwrite a user-owned legacy skill (commit 8c0328f):

  • _register_skills() gains an optional only_skill_names filter; removal passes the directory's managed names for the active directory. Install and rescaffold behavior is unchanged.
  • New regression test_remove_base_preset_preserves_user_owned_legacy_skill: fails on the previous head (1239cbe), passes now.
  • tests/specify_cli/presets, tests/integrations, tests/specify_cli/integrations, tests/specify_cli/extensions, tests/specify_cli/bundles, tests/test_presets.py, tests/test_extension_skills.py: 6096 passed, 9 skipped. uvx ruff@0.15.0 check src tests: clean.

AI disclosure: written and pushed by Claude Code (model: Claude Opus 5.5, max reasoning effort, autonomous agent mode); the code, test and this comment are AI-generated.

🤖 Addressed by Claude Code

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

triage-must-have Verdict: high-value, important work for Spec Kit — do first

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants